Skip to content

chore: Upgrade stackable-operator to 0.110.1#137

Open
siegfriedweber wants to merge 15 commits intomainfrom
chore/upgrade-stackable-operator-0.110.1
Open

chore: Upgrade stackable-operator to 0.110.1#137
siegfriedweber wants to merge 15 commits intomainfrom
chore/upgrade-stackable-operator-0.110.1

Conversation

@siegfriedweber
Copy link
Copy Markdown
Member

@siegfriedweber siegfriedweber commented Apr 27, 2026

Description

Part of stackabletech/issues#844

Bump stackable-operator to 0.110.1:

  • Replaced the untyped HashMap<String, HashMap<String, String>> config overrides with a typed OpenSearchConfigOverrides struct that only accepts opensearch.yml.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Reviewer

  • Code contains useful comments
  • Code contains useful logging statements
  • (Integration-)Test cases added
  • Documentation added or updated. Follows the style guide.
  • Changelog updated
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

  • Feature Tracker has been updated
  • Proper release label has been added
  • Links to generated (nightly) docs added
  • Release note snippet added
  • Add type/deprecation label & add to the deprecation schedule
  • Add type/experimental label & add to the experimental features tracker

@siegfriedweber siegfriedweber marked this pull request as ready for review April 30, 2026 15:27
@siegfriedweber siegfriedweber requested a review from a team April 30, 2026 15:28
@siegfriedweber siegfriedweber moved this to Development: Waiting for Review in Stackable Engineering Apr 30, 2026
@siegfriedweber siegfriedweber marked this pull request as draft May 4, 2026 06:44
@siegfriedweber siegfriedweber moved this to Development: Waiting for Review in Stackable Engineering May 4, 2026
@siegfriedweber siegfriedweber marked this pull request as ready for review May 4, 2026 06:51
@sbernauer sbernauer requested a review from dervoeti May 4, 2026 07:15
@sbernauer sbernauer moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering May 4, 2026
@siegfriedweber siegfriedweber removed the request for review from a team May 6, 2026 08:57
Copy link
Copy Markdown
Member

@dervoeti dervoeti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically LGTM, but, regarding consistency across our platform, this PR has a few things that differ from the decision:

  • No support for JsonPatches
  • UserProvided(Value) instead of UserProvided(String)
  • KeyValueConfigOverrides use BTreeMap<String, Option<String>> instead of BTreeMap<String, String> (to delete values?)

There are good reasons to do this in this PR, it's cool that Role/RoleGroup config overrides are merged, but for users it might be confusing that some overrides are defined differently for OpenSearch compared to other products.

I'm raising this now because at the moment we could still change the decision (and the implementation in operator-rs) as the other typed config overrides PRs have not been released yet.

@sbernauer @siegfriedweber any opinions on this? For each point, we can decide to live with the difference or to adapt either opensearch-operator or operator-rs.
I can live with not supporting JsonPatches in the OpenSearch operator. But I'd like UserProvided to take a string to that it's always the same way across all types of config files and a user can just paste the config in there. On the other hand it's hopefully not commonly used so it's not that important.
No strong opinion about KeyValueConfigOverrides, maybe we could use BTreeMap<String, Option<String>> in operator-rs as well so OpenSearch operator does not have to define its own struct?

@siegfriedweber
Copy link
Copy Markdown
Member Author

siegfriedweber commented May 7, 2026

  • No support for JsonPatches

There are several problems with JsonPatch:

  • It is not an associative operation: (base × role-patch) × role-group-patch ≠ base × (role-patch × role-group-patch)
    I would expect that the patch defined at the role level is applied to the base configuration before the patch defined at the role group level is. But the operators are actually implemented to first merge the configOverrides from the role and role group level (where keys in the role group level override those in the role level) and afterwards apply them to the base configuration. As the other variants of configOverrides are associative, this is just an implementation detail, but JsonPatch would produce unexpected results.
  • A JsonPatch can fail, the other variants cannot. This poses a problem for the structure in the OpenSearch operator. The validation step should reject an invalid cluster specification. But it cannot be determined beforehand if a patch can be applied. It would fail in the build step, which is currently infallible.
  • A JsonPatch highly depends on the base configuration, but the base configuration should be an implementation detail. JsonMergePatch and KeyValueConfigOverrides are more resilient to changes.

I can't think of an OpenSearch setting where a JsonPatch would make sense, but there are a lot of pitfalls, so I would leave it out of the OpenSearch operator. We can also reconsider the decision, especially if the other operators are aligned to the structure of the OpenSearch operator.

The other operators currently only use KeyValueConfigOverrides. Therefore, the users will not recognize that the JsonConfigOverrides are implemented differently in this operator.

@dervoeti
Copy link
Copy Markdown
Member

dervoeti commented May 7, 2026

The other operators currently only use KeyValueConfigOverrides

OPA uses JsonConfigOverrides, but I'd be fine with dropping JsonPatch there as well. @sbernauer I think you had some use cases for JsonPatch though, not sure. I'm fine with either option (dropping JsonPatch completely or supporting it only for some operators), I see the headaches it can cause (as you explained), just wanted to discuss now explicitly which way we want to go and then maybe update the decision.

@siegfriedweber
Copy link
Copy Markdown
Member Author

UserProvided(Value) instead of UserProvided(String)

To clarify, UserProvided is only used for JSON or YAML files.

Value has the following advantages:

  • It ensures that the content is valid JSON/YAML. If a String is used then we have to decide if the operator should try to parse it or if it should just be written as is to the configuration file. In the latter case, it depends on the product how prominently the user is informed or if the error is just ignored.
  • What to do if UserProvided(String) is used at the role level and JsonMergePatch at the role group level? They cannot be merged if the string is not parsed in the operator. If it is parsed, then UserProvided(Value) is the better option.

a user can just paste the config in there

There should be no difference between Value and String.

@siegfriedweber
Copy link
Copy Markdown
Member Author

KeyValueConfigOverrides use BTreeMap<String, Option<String>> instead of BTreeMap<String, String> (to delete values?)

This is just an implementation detail. BTreeMap<String, Option<String>> implements Merge by default,
BTreeMap<String, String> does not. I could have written the implementation for Merge manually, but it should make no difference for the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Development: In Review

Development

Successfully merging this pull request may close these issues.

3 participants